First-time contributor. (Noob level)
Anything I'm missing out on?

Thanks for pushing me to get into this!
@thefourtheye

CI: https://ci.nodejs.org/job/node-test-pull-request/7455/

Good job @adityaanandmc 👏

@adityaanandmc Changes are all good. If you could make sure if the commits follow these guidelines it would be great?

@thefourtheye Do the changes look alright ?

@thefourtheye @cjihrig
Are we allowed to do this? Create a server and kill it simply to get a free port.
Is there a way to just get an unused port ?

Are we allowed to do this? Create a server and kill it simply to get a free port.

That would probably introduce a race condition. Once you close the server to release the port, something else can claim that port.

@cjihrig What would be the ideal way to get this done? I was aware of the race condition and hence a little stuck. My commit ( code below ) will fail if the race condition is met.
How do I secure a free port and listen to it ensuring that a server hasn't been created in the gap?

const net = require('net');
const assert = require('assert');

//const c = net.createConnection(common.PORT);
const server = net.createServer();
server.listen(0);
const port = server.address().port;
server.close();

const c = net.createConnection(port);

c.on('connect', common.mustNotCall());

c.on('error', common.mustCall(function(e) {
    assert.strictEqual(e.code, 'ECONNREFUSED');
    //assert.strictEqual(e.port, common.PORT);
    assert.strictEqual(e.port, port);
    assert.strictEqual(e.address, '127.0.0.1');
}));

@adityaanandmc wrote

How do I secure a free port and listen to it ensuring that a server hasn't been created in the gap?

Although the possibility of this happening is less, it is still possible and we cannot avoid it.

@adityaanandmc what about doing something like this:

...
const server = net.createServer();
server.listen(0);
const port = server.address().port;
const c = net.createConnection(port);
server.close();
...

@santigimeno Now i feel like a idiot :P Thanks for that!! :D

@adityaanandmc could you squash the commits into one? Thanks!

CI: https://ci.nodejs.org/job/node-test-pull-request/7523/

@santigimeno will squash them!!
Looks like there are a few failures test-net-connect-immediate-finish

I can't seem to understand the failures. I thought about doing this, but it didn't seem like the right thing to do.

const server = net.createServer();
server.listen(0);
const port = server.address().port;
server.close();

const client = net.connect({host: '***', port: port});

@adityaanandmc looking at the changes again it seems that we're trying to connect() to the 0 port. I don't think that makes sense: the port 0 makes sense when calling bind() or listen() not connect(). What I think it could be done is create a helper function in common.js (connectToRandomPort for example) to reserve a port and connect, similar to the code you were proposing.

@santigimeno @thefourtheye I did that and it passed locally but at times it fails with the following error!
It started only after i had touched this file test-net-connect-immediate-finish.
Not able to figure out why it's happening.

Path: parallel/test-domain-no-error-handler-abort-on-uncaught-0
Command: out/Release/node /Users/adianand/Labs/node/test/parallel/test-domain-no-error-handler-abort-on-uncaught-0.js
--- TIMEOUT ---
=== release test-domain-no-error-handler-abort-on-uncaught-1 ===
Path: parallel/test-domain-no-error-handler-abort-on-uncaught-1
Command: out/Release/node /Users/adianand/Labs/node/test/parallel/test-domain-no-error-handler-abort-on-uncaught-1.js
--- TIMEOUT ---
=== release test-domain-no-error-handler-abort-on-uncaught-2 ===
Path: parallel/test-domain-no-error-handler-abort-on-uncaught-2
Command: out/Release/node /Users/adianand/Labs/node/test/parallel/test-domain-no-error-handler-abort-on-uncaught-2.js
--- TIMEOUT ---
=== release test-domain-no-error-handler-abort-on-uncaught-3 ===
Path: parallel/test-domain-no-error-handler-abort-on-uncaught-3
Command: out/Release/node /Users/adianand/Labs/node/test/parallel/test-domain-no-error-handler-abort-on-uncaught-3.js
--- TIMEOUT ---
=== release test-domain-no-error-handler-abort-on-uncaught-4 ===
Path: parallel/test-domain-no-error-handler-abort-on-uncaught-4
Command: out/Release/node /Users/adianand/Labs/node/test/parallel/test-domain-no-error-handler-abort-on-uncaught-4.js
--- TIMEOUT ---

Sorry for flooding you with doubts. Just a lil confused for a first-time contributor on a project of this scale.

I think the actual error in the output you linked to is this:

not ok 357 sequential/test-benchmark-child-process
  ---
  duration_ms: 60.190
  severity: fail
  stack: |-
    timeout

That is unrelated, and was tracked at #12817 (but should be fixed now).

Either way, it looks like this PR has a merge conflict now so it might need to rebase – is that something you can do?

@adityaanandmc, what @addaleax said.

BTW, could you address this nit? Thanks!

Sure thing !! Will do that in a bit though. ( heading out to watch Guardians of the Galaxy ) :D

@addaleax @santigimeno Rebased!! Thanks for that. Can a CI job be triggered?

CI: https://ci.nodejs.org/job/node-test-commit/9717/

@adityaanandmc sorry to bring this up again, but it would be great if you could address this. Anyway, the PR as it is LGTM. Thanks!

@santigimeno That's alright. I did not see that comment until now. Sorry about that!
Added comments ! Do they look alright ?

Updated:

• test-net-better-error-messages-port-hostname
• test-net-options-lookup.js

Thanks @santigimeno @thefourtheye @Trott @cjihrig !!

Added comments ! Do they look alright ?

Yes, LGTM. I think another in test-net-localerror.js could be useful. Thanks!

@santigimeno Done!

@thefourtheye Done. CI Build trigger necessary ?

CI Run: https://ci.nodejs.org/job/node-test-pull-request/7965/

Landed in 94eed0f